fix: parsing of .npmrc file#115
Conversation
| console.log(`Found existing user .npmrc file at ${userNpmrcPath}`); | ||
|
|
||
| // Parse the `.npmrc` content using the `npm/ini` package. | ||
| const npmConfig = ini.parse(fs.readFileSync(userNpmrcPath, "utf-8")); |
There was a problem hiding this comment.
We use ini.parse to take care of parsing the config (all whitespaces are stripped out)
| let hasAuthToken = false; | ||
| for (const [key, value] of Object.entries(npmConfig)) { | ||
| if (/\/\/(.*)authToken$/.test(key) && Boolean(value)) { | ||
| console.log("The .npmrc file has an authToken"); | ||
| hasAuthToken = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the "new logic" to test the presence of the authToken. The regex also can be simpler.
| console.log( | ||
| "The .npmrc file does not have an authToken defined, creating one using the `NPM_TOKEN` environment variable" | ||
| ); | ||
| if (!processEnv.NPM_TOKEN) { |
There was a problem hiding this comment.
I thought it would make sense to throw an error if the NPM_TOKEN is not present.
There was a problem hiding this comment.
| npmConfig["//registry.npmjs.org/:_authToken"] = processEnv.NPM_TOKEN; | ||
| fs.writeFileSync(userNpmrcPath, ini.stringify(npmConfig)); |
There was a problem hiding this comment.
We can assign new values to the config object and then let npm/ini serialize the content.
|
Thank you for the PR! This refactor might be worth it but it will require some further analysis - if you are fine with waiting then we can leave it as is and I will get back to it this week. I would prefer to merge a small PR, that would only fix the whitespace issue, first. That could hit the stable release sooner. |
|
Sure, I opened a separate PR with only a quick fix for the regex: #116 |
| if (!processEnv.NPM_TOKEN) { | ||
| throw new Error( | ||
| "Missing NPM authToken. Please make sure you have the `NPM_TOKEN` environment variable defined." | ||
| ); | ||
| } |
There was a problem hiding this comment.
This should be removed. There is also a possibility to have .npmrc file in the project itself - it takes precedence over the user-level .npmrc and this should be respected. The current logic is that we try to add token to the user-level .npmrc which should be harmless if there is a project-level .npmrc because it will "shadow" over the user-level .npmrc anyway.
There was a problem hiding this comment.
Hmm ok but then we end up writing an "empty/undefined" authToken value. Is that what we want?
There was a problem hiding this comment.
No, probably not - we can skip over creating/editing .npmrc when NPM_TOKEN is not set
|
|
||
| let hasAuthToken = false; | ||
| for (const [key, value] of Object.entries(npmConfig)) { | ||
| if (/\/\/(.*)authToken$/.test(key) && Boolean(value)) { |
There was a problem hiding this comment.
We should tighten up this regex - one, in theory, can have defined multiple auth tokens, for multiple registries. If one has a token for registry foo then we should still create an entry for the npm registry (if it's missing)
emmenko
force-pushed
the
nm-fix-npmrc-parse-token
branch
from
e1e1667 to
d1da8fa
Compare
| - version - The command to update version, edit CHANGELOG, read and delete changesets. Default to `changeset version` if not provided | ||
| - commit - The commit message to use. Default to `Version Packages` | ||
| - title - The pull request title. Default to `Version Packages` | ||
| - skipNpmrcCheck - Check if there is a user-defined `.npmrc` file with a proper npm registry and an `authToken`. If not, a default `.npmrc` file is created. |
There was a problem hiding this comment.
@Andarist I added this option to opt-out of the .npmrc checks.
I find it useful in case I don't need the use the default npm registry (for example when using github packages).
What do you think?
| import * as ini from "ini"; | ||
|
|
||
| // https://docs.npmjs.com/using-private-packages-in-a-ci-cd-workflow#create-and-check-in-a-project-specific-npmrc-file | ||
| const npmRegistryTokenKey = "//registry.npmjs.org/:_authToken"; |
There was a problem hiding this comment.
I did some research and couldn't find a case where the value isn't exactly like this (for the npm registry). Therefore I think we can omit the regex and simply do a 1:1 check.
| fs.writeFileSync(userNpmrcPath, ini.stringify(npmConfig)); | ||
| } else { | ||
| console.warn( | ||
| "Missing `NPM_TOKEN` environment variable, skipping update of .npmrc file." |
There was a problem hiding this comment.
So now in case the NPM_TOKEN is missing, we simply log a warning.
|
@Andarist Hey, I updated the PR based on the feedback. I also added a new option Let me know how you prefer to proceed here, thanks! |
- Fixed 10 security alerts (0 critical, 4 high, 6 medium, 0 low) - Direct upgrades: @actions/github 6.0.1 → 8.0.1 (undici 5.29.0 → 6.25.0) - Scoped resolutions added: picomatch ^2.3.2, minimatch ^3.1.3, uuid ^14.0.0, js-yaml ^3.14.2 - Resolution updates: @octokit/core ^7.0.0, undici ^6.24.0 - Jest configuration: Added ES module support and uuid mock for test compatibility - Validation: Build passes, tests pass with proper ES module handling Alerts fixed: - changesets#105, changesets#110, changesets#111, changesets#112, changesets#113 (undici): WebSocket DoS, request smuggling, CRLF injection - changesets#108 (minimatch): ReDoS via GLOBSTAR segments - changesets#114, changesets#115 (picomatch): ReDoS via extglob, method injection - changesets#116 (uuid): Buffer bounds check issue - changesets#104 (js-yaml): Prototype pollution in merge Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Fixes #114
I decided to use the
npm/inipackage to take care of the parse/stringify part of the.npmrcconfig (which is what NPM itself uses). This avoids us having to deal with whitespaces etc.To make it easier to test, I extracted the logic into a function in a separate file
npmUtils.